Skip to content

Always call getCandidateForOverloadFailure - #28564

Closed
Deleted user (ghost) wants to merge 1 commit into
masterfrom
getCandidateForOverloadFailure
Closed

Always call getCandidateForOverloadFailure#28564
Deleted user (ghost) wants to merge 1 commit into
masterfrom
getCandidateForOverloadFailure

Conversation

@ghost

Copy link
Copy Markdown

This would make it so the command line compiler and services don't differ in the overload chosen.

@ghost
ghost force-pushed the getCandidateForOverloadFailure branch from db2a202 to 1738ee0 Compare November 16, 2018 18:21
@sandersn

Copy link
Copy Markdown
Member

Wesley Wigham (@weswigham) Should we be worried about performance problems in services from merging this? resolveUntypedCall is way simpler than getCandidateForOverloadFailure. Unfortunately we don't have an easy way to measure perf differences for a PR there.

(Andrew Casey (@amcasey) in case this is a scenario you want to take note of.)

@weswigham

Copy link
Copy Markdown
Member

Should we be worried about performance problems in services from merging this?

The reverse - you should be concerned that command line performance might suffer; the language service checker has always been using the more expensive getCandidateForOverloadFailure - that's how it still provides completions, even in the presence of errors; unlike the command line compiler, it doesn't immediately replace the result with any.

Since that's the concern, a simple perf test on this PR should bear out if that's really a problem.

@sandersn

Copy link
Copy Markdown
Member

Oh, yeah, I misread produceDiagnostics. I'll merge with master and run a perf test then.

@weswigham

Wesley Wigham (weswigham) commented Feb 6, 2020

Copy link
Copy Markdown
Member

Do note that this is technically a prerequisite for #28584 (which is probably why you're here), which we'd like to be able to take to reduce services memory usage~

@sandersn

Copy link
Copy Markdown
Member

Ha ha nope. I'm just going through Pall Mall by order of age. #28584 is next.

@sandersn

Copy link
Copy Markdown
Member

Closing in favour of #36665 since I couldn't find the branch for this PR.

Nathan Shively-Sanders (sandersn) added a commit that referenced this pull request Feb 7, 2020
@RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) deleted the getCandidateForOverloadFailure branch October 25, 2022 20:18
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
@typescript-bot

Copy link
Copy Markdown
Contributor

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants